Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Large-scale refactoring #63

Merged
merged 13 commits into from
Jan 7, 2019
Merged

Large-scale refactoring #63

merged 13 commits into from
Jan 7, 2019

Conversation

aendra-rininsland
Copy link
Member

This does a llllllllot.

  1. Upgrades to Babel 7
  2. Removes all unnecessary data joins
  3. Removes a lllllllot of superfluous code
  4. Removes the watermark g element if no watermark is present (breaking change)
  5. Renames all instances of colour to color (breaking change)
  6. Ensures each element is only created once even if frame is called multiple times (possibly breaking change; element values must be set before frame is first called).

Fixes #57; fixes #52; fixes #45.

Ændrew Rininsland added 6 commits March 23, 2018 17:06
🐛 Fixes #45.
25512a1 
- Replaces (most) calls to Selection.html with Selection.text
- Wraps any remaining HTML doctype-specific DOM manip in if statements
- Adds a test ensuring a pure SVG context can be rendered with JSDom
Removes unnecessary transitions from chartframe.js
0c2e1a4
Removed half the stupid .data() calls
f2d9653
All useless selections removed hurrah 🎉
6283f37
Removes dependency on d3-transition
e031c7f
Possibly breaking -- this should probably be sourceStyle, not subtitl…
e5f77ad 
…eStyle
@aendra-rininsland
Copy link
Member Author

aendra-rininsland commented Dec 14, 2018 •
edited
Loading

Coverage is now ≅99%. I've also reinstated backgroundColour but made it throw a deprecation notice.

Ændrew Rininsland and others added 3 commits December 14, 2018 14:28
Merge branch 'refactoring' into works-in-jsdom
cdfa143
Reinstating pure SVG context test.
076566a
🐛 Fixes #45. (#46)
caeaa22 
- Replaces (most) calls to Selection.html with Selection.text
- Wraps any remaining HTML doctype-specific DOM manip in if statements
- Adds a test ensuring a pure SVG context can be rendered with JSDom
@aendra-rininsland
Copy link
Member Author

aendra-rininsland commented Dec 14, 2018 •
edited
Loading

Updated and merged the "pure SVG context" PR (#46) into this one.

joannaskao
joannaskao reviewed Dec 19, 2018
View reviewed changes
readme.md Outdated
@@ -99,12 +99,12 @@ A convenience setter: Set as many values as you choose by passing a big object t

This get the frame to try and automatically comput its top margin. _Not really reccomended though it it used by the pre-packaged webframes_.

<a id="frame-backgroundColour" href="#frame-backgroundColour">#</a>frame.**backgroundColour(_string_)**
<a id="frame-backgroundColor" href="#frame-backgroundColor">#</a>frame.**backgroundColor(_string_)**

Set the background colour of the frame. For the single argument you can use the same css color naming schemes that you use in HTML, whether that's color names (that is red), rgb values (that is rgb(255,0,0)), hex values, rgba values, etc. If no argument is specified returns the current value.
Copy link
Contributor

@joannaskao joannaskao Dec 19, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's change colour in "background colour of the frame" to color for consistency.

@joannaskao
Copy link
Contributor

joannaskao commented Dec 19, 2018 •
edited
Loading

For consistency, I think we should take out this line: https://github.com/Financial-Times/g-chartframe/blob/refactoring/src/videoframe.js#L10

joannaskao
joannaskao approved these changes Dec 19, 2018
View reviewed changes
Copy link
Contributor

@joannaskao joannaskao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for doing this!

Good in-person suggestion to wait until the new year to merge this in.

@aendra-rininsland aendra-rininsland merged commit 05324c2 into master Jan 7, 2019
@aendra-rininsland aendra-rininsland deleted the refactoring branch January 7, 2019 09:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joannaskao joannaskao joannaskao approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

There is no U in "color" Weird data binding issue Should work in a pure SVG JSDom context
2 participants
@aendra-rininsland @joannaskao